Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle multiple file changes. #296

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Handle multiple file changes. #296

wants to merge 2 commits into from

Conversation

wclr
Copy link
Contributor

@wclr wclr commented Jun 7, 2022

Without it if multiple changes occured at once, it will try to start process multiple times.

Without it if multiple changes occured at once, it will try to start process multiple times.
@wclr
Copy link
Contributor Author

wclr commented Jan 11, 2023

@bjornstar are you going to review this somehow?

@reslear
Copy link

reslear commented Jan 11, 2023

@bjornstar see please)

@bjornstar
Copy link
Collaborator

I see test failures, and no tests to validate the behavior.

@wclr
Copy link
Contributor Author

wclr commented Jan 12, 2023

Well, I've tried to reproduce the problem with tests, and could not: watcher.removeAll() in the handler removes watching after just the first change. So the next change is not caught and handled.

I'm pretty sure that in my case I see that multiple file changes happened at once resulted in multiple Restarting for each file. My case is dev app that is running in docker (which requires polling) and it is watching for quite a lot of files. And simultaneous change happen due to re-compilation and emitting a great amount of new files. So, I guess, this could probably occur because watcher removal is not completed to prevent sequential change events from firing and handling. And setting isPaused right after the first change, and then checking it for subsequent change solved this problem (I'm currently using a modified version to avoid this).

Btw if to add:

watcher.on('change', file => {
   if (isPaused) return;
   isPaused = true;

I don't see any current tests failing.

@wclr
Copy link
Contributor Author

wclr commented Jan 16, 2023

@bjornstar So it seems that this case is is not easy to reproduce in tests. But logically it should be understood that this:

watcher.on('change', file => {
   if (isPaused) return;
   isPaused = true;

allows to prevent consequent changes that should not be handled.

What do you think?

@reslear
Copy link

reslear commented Apr 28, 2023

hi @bjornstar, i use this PR as a patch and test:

  • save many files at the same time
  • switch between branches with many file changes

and works fine 🔥 prevents a lot of unnecessary restarts.

@wclr good work

@wclr
Copy link
Contributor Author

wclr commented Dec 26, 2023

@bjornstar Look at this again please, this is a simple logical change that we will NOT handle any changes as soon as isPaused set, currently it is possible that multiple file changes will pass without such filter and try to restart.

Current:

  watcher.on('change', file => {
    clearOutput();
    notify('Restarting', `${file} has been modified`);
    watcher.removeAll();
    isPaused = true;
    if (child) {
      // Child is still running, restart upon exit
      child.on('exit', start);

With change:

watcher.on('change', file => {
    if (isPaused) return;
    isPaused = true;
    clearOutput();
    notify('Restarting', `${file} has been modified`);
    watcher.removeAll();    
    if (child) {
      // Child is still running, restart upon exit
      child.on('exit', start);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants